Size Change: +216 B (+0.01%)

Total Size: 1.78 MB

_{compressed-size-action}

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: andrewserong <[email protected]>
Co-authored-by: aristath <[email protected]>
Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: ciampo <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

I think this might be related to #55755 as well...

Thanks for the review!

Trunk doesn't disable className support. This might be the cause of the bug noted in #64232 (comment)

This was probably the cause 😅 I removed this and the fixture was generated as expected.

The last thing I noticed is that the original deprecation's fixtures weren't updated. All deprecations have to bring that version of the block up to the latest markup.

This PR only updates markup if the type attribute is radio or checkbox. But the type attribute of the v1 fixture is text. I think this is the reason why the serialized v1 fixture is not updated.

This PR only updates markup if the type attribute is radio or checkbox. But the type attribute of the v1 fixture is text. I think this is the reason why the serialized v1 fixture is not updated.

Thanks for the explanation, it probably means we're missing some fixtures to cover all bases for the block.

I have included blocks with text, radio and checkbox attributes in both v1 and v2. All deprecations appear to work as expected.

Sounds like a good idea!

I've split it into all available types as variations, but should we remove fixtures that aren't affected by the deprecation?

Flaky tests detected in 8db2101.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10413264875
📝 Reported issues:

• [Flaky Test] Redirection after template creation #60678 in /test/e2e/specs/site-editor/site-editor-url-navigation.spec.js

I've split it into all available types as variations, but should we remove fixtures that aren't affected by the deprecation?

Sorry I wasn't more clear with my suggestion @t-hamano.

I was only suggesting that we had a default or base fixture for each of the different form input types. The deprecation fixtures can be specific to the use case or type they are relevant for e.g. the v2 deprecation fixture could only be a checkbox and/or radio as that's the main focus of the change.

Some of the input types are just a change of attribute on the input element so maybe those those don't need their own fixture e.g. url, tel etc.

I'm probably missing some but maybe we just need base fixtures for:

• core__form-input.html (text type, which also pretty much covers url, tel etc)
• core__form-input__checkbox.html
• core__form-input__radio.html
• core__form-input__textarea.html

Does that help clarify things? If not I'm happy to try again 😅

I reorganized it into the following four fixtures.

Are there any fixtures that we don't need? Sorry if I didn't understand 😅

• core__form-input__deprecated-v1.html
• core__form-input__deprecated-v2.html
• core__form-input__radio__deprecated-v1.html
• core__form-input__radio__deprecated-v2.html
• core__form-input__checkbox__deprecated-v1.html
• core__form-input__checkbox__deprecated-v2.html
• core__form-input__textarea__deprecated-v1.html
• core__form-input__textarea__deprecated-v2.html

Sorry if I didn't understand 😅

It's probably more that I'm not explaining the fixtures I'm proposing very well, without listing every file out individually.

Are there any fixtures that we don't need?

After the latest change, we're still missing the base fixtures for the different input types as outlined in #64232 (comment).

core__form-input.html (text type, which also pretty much covers url, tel etc)
core__form-input__checkbox.html
core__form-input__radio.html
core__form-input__textarea.html

These cover the latest (current) version of the block.

The v2 deprecation was only for checkbox and radio inputs, correct? So the deprecation fixtures for v2 probably don't need ones for textarea or other input types that aren't effected. This what I meant by this comment:

The deprecation fixtures can be specific to the use case or type they are relevant for e.g. the v2 deprecation fixture could only be a checkbox and/or radio as that's the main focus of the change.

We don't have to follow my recommendation if you don't agree with it. If you do, what I think is left is to:

1. Create base fixtures for a generic text input, checkbox, radio, and textarea. These are not fixtures for deprecations, just the current block version.
2. Remove the deprecation fixtures for input types unrelated to the deprecation. If a block's markup isn't changed from the base fixture, it's already covered for the deprecation.

If this isn't making things clearer, I can carve out some time early next week to create a diff or push a commit here if it helps.

I removed the fixtures that I deemed unnecessary. The current fixtures consist of:

• v1: This deprecation is required for all form element types because it adds the useBlocksProps to the save function (#56507)
  • core__form-input__deprecated-v1.html: Includes default, email, number, tel, url types
  • core__form-input__radio__deprecated-v1.html
  • core__form-input__checkbox__deprecated-v1.html
  • core__form-input__textarea__deprecated-v1.html
• v2: This is the target of this PR, swapping the input and label elements. This deprecation only affects the radio and checkbox types.
  • core__form-input__radio__deprecated-v2.html
  • core__form-input__checkbox__deprecated-v2.html

If I'm still misunderstanding, feel free to push 🙏

Thanks @t-hamano

1. Create base fixtures for a generic text input, checkbox, radio, and textarea. These are not fixtures for deprecations, just the current block version.

⬆️ This is the bit still missing

If I'm still misunderstanding, feel free to push 🙏

Unfortunately, I might not be able to get to this before I'm AFK until the 10th September.

@aaronrobertshaw Thanks for the reply.

This PR is not something that needs to be rushed, so I would be happy if you could take a look at it again when you have time.

@t-hamano I've rebased this and pushed some base fixtures for the various form input types. I've also aligned the default fixture with how you combined all the text, email, number, tel, url inputs in the deprecation fixtures.

Given I've contributed the last couple of commits, I'd like to get a fresh set of eyes on this PR for final approval.

@andrewserong if you can spare a moment to review this, that would be greatly appreciated 🙏

From my end, I think this should be good to go!

@aaronrobertshaw Thanks for the update! By looking at your commits, I was able to understand your intentions.

In testing this locally I noticed that the removal of the CSS rule means that any unmigrated content that is already on a site will continue to display in the other order until going in to a post and updating / saving so that the migration is run.

This is likely to occur for all blocks whose save function has been updated.

I know some approaches that leave CSS in place for backward compatibility, but I don't know of any clear standards for how much old markup should be supported with CSS.

Personally, I feel that for form blocks, it's enough to just ensure that the block doesn't break.

If we want to add CSS that takes old markup into account, you can use the following approach:

// The old markup, i.e. if there is an input element after the label content, reverse the order as before.
.wp-block-form-input__label:has(.wp-block-form-input__label-content + input[type="checkbox"]) {
	flex-direction: row-reverse;
}

This is likely to occur for all blocks whose save function has been updated.
I know some approaches that leave CSS in place for backward compatibility, but I don't know of any clear standards for how much old markup should be supported with CSS.

From my perspective, if it's a block that's used in production, it probably either needs CSS that we keep around pretty much forever (as we can't guarantee that old post content on big sites will ever be migrated), or use a server-side approach to dynamically update old markup when the block is rendered.

For the Form blocks, since it's behind an experiment, I imagine it's not so important because the blocks aren't being used in production anywhere? I'm not certain of this assumption, though, so I'll just ping @WordPress/gutenberg-core for visibility on that question.

From my perspective, if it's a block that's used in production, it probably either needs CSS that we keep around pretty much forever (as we can't guarantee that old post content on big sites will ever be migrated), or use a server-side approach to dynamically update old markup when the block is rendered.

Yes, we should keep even the CSS for changes in blocks for back compat.

Personally, I feel that for form blocks, it's enough to just ensure that the block doesn't break.

I'd echo this, since it's an experiment.. Also if the css rule is something really simple and would keep the old version as is, I don't see why not add it.

Thank you for the feedback, everyone! I've added some CSS for the old markup.

I've also updated the Testing Instructions section so you can test it.

@t-hamano , I was reading the PR description and got confused by the "From" -> "To" code snippet in the "How" section, since the two snippets are identical. Is that a typo? We should update it so that folks visiting this PR can have a better comprehension of its changes :) Thank you!!

@ciampo Thanks for letting me know! That was definitely a typo. I updated it correctly.

Thanks for the patience and iteration here

+1 — my apologies, I meant to come back to this PR and it totally slipped my mind. Thanks for all the follow-up!

Thank you everyone for the reviews 🙇‍♂️